[RISCV] Implement getIPRACSRegs hook #125586
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch implements the getIPRACSRegs hook for riscv, similar to how it's done for x86 in 14b567d. Differently from the x86 hook, in this patch we only save ra (x1) and not the frame pointer. I'm still working on a test case that shows the need to save the fp. The fixed test case was extracted from the gcc torture suite, but the same issue is found in SPEC's xz.
Signed-off-by: Mikhail R. Gadelha <[email protected]>
Member
|
@llvm/pr-subscribers-backend-risc-v Author: Mikhail R. Gadelha (mikhailramalho) ChangesFixes #124932. This patch implements the getIPRACSRegs hook for RISC-V, similar to its introduction for x86 in commit 14b567d. This hook is necessary for correct code generation when Interprocedural Register Allocation (IPRA) is enabled, ensuring that the return address register (ra / x1) is correctly saved and restored when needed. Unlike the x86 implementation, this patch only saves ra and does not yet include the frame pointer (fp). Further investigation is required to determine whether fp should also be preserved in all cases. The test case is representative of a miscompile observed in the GCC torture suite (20090113-3.c), though similar failures occur in SPEC’s xz benchmark. Full diff: https://github.com/llvm/llvm-project/pull/125586.diff 4 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVCallingConv.td b/llvm/lib/Target/RISCV/RISCVCallingConv.td
index ad06f477437702..98e05b7f8eca7c 100644
--- a/llvm/lib/Target/RISCV/RISCVCallingConv.td
+++ b/llvm/lib/Target/RISCV/RISCVCallingConv.td
@@ -42,6 +42,8 @@ def CSR_ILP32D_LP64D_V
// Needed for implementation of RISCVRegisterInfo::getNoPreservedMask()
def CSR_NoRegs : CalleeSavedRegs<(add)>;
+def CSR_IPRA : CalleeSavedRegs<(add X1)>;
+
// Interrupt handler needs to save/restore all registers that are used,
// both Caller and Callee saved registers.
def CSR_Interrupt : CalleeSavedRegs<(add X1, (sequence "X%u", 5, 31))>;
diff --git a/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp b/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
index b0a52698c1e9f1..7a99bfd1b25122 100644
--- a/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
@@ -55,6 +55,11 @@ RISCVRegisterInfo::RISCVRegisterInfo(unsigned HwMode)
: RISCVGenRegisterInfo(RISCV::X1, /*DwarfFlavour*/0, /*EHFlavor*/0,
/*PC*/0, HwMode) {}
+const MCPhysReg *
+RISCVRegisterInfo::getIPRACSRegs(const MachineFunction *MF) const {
+ return CSR_IPRA_SaveList;
+}
+
const MCPhysReg *
RISCVRegisterInfo::getCalleeSavedRegs(const MachineFunction *MF) const {
auto &Subtarget = MF->getSubtarget<RISCVSubtarget>();
diff --git a/llvm/lib/Target/RISCV/RISCVRegisterInfo.h b/llvm/lib/Target/RISCV/RISCVRegisterInfo.h
index 3ab79694e175c8..6c4e9c7b1bdc7f 100644
--- a/llvm/lib/Target/RISCV/RISCVRegisterInfo.h
+++ b/llvm/lib/Target/RISCV/RISCVRegisterInfo.h
@@ -62,6 +62,8 @@ struct RISCVRegisterInfo : public RISCVGenRegisterInfo {
const MCPhysReg *getCalleeSavedRegs(const MachineFunction *MF) const override;
+ const MCPhysReg *getIPRACSRegs(const MachineFunction *MF) const override;
+
BitVector getReservedRegs(const MachineFunction &MF) const override;
bool isAsmClobberable(const MachineFunction &MF,
MCRegister PhysReg) const override;
diff --git a/llvm/test/CodeGen/RISCV/ipra.ll b/llvm/test/CodeGen/RISCV/ipra.ll
index 717b778bcde375..761d4772283367 100644
--- a/llvm/test/CodeGen/RISCV/ipra.ll
+++ b/llvm/test/CodeGen/RISCV/ipra.ll
@@ -68,21 +68,25 @@ define internal void @foobar(ptr %live_throughout.0.val) norecurse nounwind {
; RV64-LABEL: foobar:
; RV64: # %bb.0: # %entry
; RV64-NEXT: addi sp, sp, -48
+; RV64-NEXT: sd ra, 40(sp) # 8-byte Folded Spill
; RV64-NEXT: mv a1, a0
-; RV64-NEXT: addi a0, sp, 16
-; RV64-NEXT: addi a2, sp, 12
+; RV64-NEXT: addi a0, sp, 8
+; RV64-NEXT: addi a2, sp, 4
; RV64-NEXT: call bmp_iter_set_init
+; RV64-NEXT: ld ra, 40(sp) # 8-byte Folded Reload
; RV64-NEXT: addi sp, sp, 48
; RV64-NEXT: ret
;
; RV32-LABEL: foobar:
; RV32: # %bb.0: # %entry
-; RV32-NEXT: addi sp, sp, -32
+; RV32-NEXT: addi sp, sp, -48
+; RV32-NEXT: sw ra, 44(sp) # 4-byte Folded Spill
; RV32-NEXT: mv a1, a0
-; RV32-NEXT: addi a0, sp, 8
-; RV32-NEXT: addi a2, sp, 4
+; RV32-NEXT: addi a0, sp, 16
+; RV32-NEXT: addi a2, sp, 12
; RV32-NEXT: call bmp_iter_set_init
-; RV32-NEXT: addi sp, sp, 32
+; RV32-NEXT: lw ra, 44(sp) # 4-byte Folded Reload
+; RV32-NEXT: addi sp, sp, 48
; RV32-NEXT: ret
;
; RV64-WITHFP-LABEL: foobar:
|
topperc
reviewed
Feb 3, 2025
topperc
approved these changes
Feb 3, 2025
Collaborator
topperc
left a comment
topperc
left a comment
There was a problem hiding this comment.
LGTM. Should we backport to LLVM 20?
asb
changed the title
Contributor
Especially this early in the cycle, no opposition to it. I will note that due to #119556 there's at least one other issue with using IPRA in practice - you can get crashes unless you use |
8 tasks
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
Fixes llvm#124932. This patch implements the getIPRACSRegs hook for RISC-V, similar to its introduction for x86 in commit 14b567d. This hook is necessary for correct code generation when Interprocedural Register Allocation (IPRA) is enabled, ensuring that the return address register (ra / x1) is correctly saved and restored when needed. Unlike the x86 implementation, this patch only saves ra and does not yet include the frame pointer (fp). Further investigation is required to determine whether fp should also be preserved in all cases. The test case is representative of a miscompile observed in the GCC torture suite (20090113-3.c), though similar failures occur in SPEC’s xz benchmark.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #124932.
This patch implements the getIPRACSRegs hook for RISC-V, similar to its introduction for x86 in commit 14b567d. This hook is necessary for correct code generation when Interprocedural Register Allocation (IPRA) is enabled, ensuring that the return address register (ra / x1) is correctly saved and restored when needed.
Unlike the x86 implementation, this patch only saves ra and does not yet include the frame pointer (fp). Further investigation is required to determine whether fp should also be preserved in all cases.
The test case is representative of a miscompile observed in the GCC torture suite (20090113-3.c), though similar failures occur in SPEC’s xz benchmark.